Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added a configuration option on how to label PCR data #662

Merged
merged 3 commits into from
Jun 30, 2021

Conversation

Mailaender
Copy link
Contributor

Closes #661

@Mailaender
Copy link
Contributor Author

Updated.

@eselmeister
Copy link
Contributor

What happens if

LabelSetting referenceLabel = LabelSetting.valueOf(preferenceStore.getString(PreferenceConstants.P_PCR_REFERENCE_LABEL));

the return of preferenceStore.getString(PreferenceConstants.P_PCR_REFERENCE_LABEL) is null or "" or doesn't match any enum name?

@Mailaender
Copy link
Contributor Author

	private String getString(Properties p, String name) {
		String value = p != null ? p.getProperty(name) : null;
		if (value == null) {
			return STRING_DEFAULT_DEFAULT;
		}
		return value;
	}

will never return null. STRING_DEFAULT_DEFAULT is an empty string. I added a fallback to the default value instead of no label. Also added enum parsing that will not crash the widget, but also go to a fallback.

I now also set the ID of line series and use the description field instead.

@eselmeister
Copy link
Contributor

eselmeister commented Jun 29, 2021

Looks good. Two comments:

A) When introducing a new dependency, I recommend to also set the required version in the MANIFEST.MF:
com.google.guava;bundle-version="27.1.0"

B) I recommend to use as less dependencies as possible. In this case, Google Enums is not necessarily needed. You could rewrite the code in the following way, which is also more compact:

private String getLabel(IWell well) {

	LabelSetting labelSetting = getLabelSetting();
	switch(labelSetting) {
		case SAMPLENAME:
			return well.getSampleId();
		case COORDINATE:
			return well.getPosition().toString();
		case COORDINATE_SAMPLENAME:
		default:
			return well.getPosition().toString() + ": " + well.getSampleId();
	}
}

private LabelSetting getLabelSetting() {

	try {
		return LabelSetting.valueOf(preferenceStore.getString(PreferenceConstants.P_PCR_REFERENCE_LABEL));
	} catch(Exception e) {
		return LabelSetting.COORDINATE_SAMPLENAME;
	}
}

@Mailaender
Copy link
Contributor Author

Rebased, removed Google Guave, initialized the setting and put the settings button on the right.

@eselmeister eselmeister merged commit 1f32463 into eclipse-chemclipse:develop Jun 30, 2021
@Mailaender Mailaender deleted the pcr-label branch June 30, 2021 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PCR data labeling misses the vital information
2 participants